IIIF V3 Working Version #1
Conversation
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @Jason-Benson. I haven't thoroughly reviewed everything yet, but this looks very good! See below for some initial comments and suggestions. It looks like I may need to do a little work to update the baseline code here so that this pull request shows ONLY changes you made and not some unrelated recent adjustments from Chris. I also think it would be helpful for you to run our code style tools (as discussed below) to apply some automatic updates; then I will be less distracted on minor style issues and can focus more on the meat of the review.
Let me know if you need any more help with anything, and I'll give this another, more thorough round of reviewing once you've had a chance to update things and/or reply to my comments below.
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @Jason-Benson, this looks really good. I appreciate the fact that you managed to make significant changes without scrambling the code too much, so it was actually easier than I had anticipated to review!
See below for a number of comments, all of a pretty nitpicky nature. Please forgive me for my detail orientation. ;-)
| $ids[] = $matches[0]; | ||
| } | ||
|
|
||
| foreach ($data['items'] as $canvas) { |
There was a problem hiding this comment.
Might be worth adding this little safety check so if 'items' is somehow undefined, the system doesn't error out.
| foreach ($data['items'] as $canvas) { | |
| foreach ($data['items'] ?? [] as $canvas) { |
This comment was marked as outdated.
This comment was marked as outdated.
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks for the progress here. I've resolved all the comments that are fully resolved and I've left a few new ones. There were a couple of individual comments from the first review that haven't been addressed yet (e.g. a comment about VuDLController, and the one about the file ownership). It's easy to miss stuff -- especially when GitHub collapses threads and helpfully hides it from you -- but please do another spin through the list and see if you can find the outstanding issues.
I also see that you replied to the email that you received from GitHub with the review notification. I'd recommend in future using the threaded conversations on the pull request to discuss individual points -- it makes it a little easier to keep track of everything (apart from the aforementioned automatic thread hiding, which drives me a little crazy). In any case, I think I was able to parse out your replies from this first response, so no need to repeat yourself. I think my new comments should answer the issues you raised in your notes. :-)
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @Jason-Benson! See below for next round of comments. In addition to that, two general points:
1.) Maybe it's worth adding the unit test to this PR once you're happy with it
2.) The files still need to be chmod 644'ed
| if($type==='image'){ | ||
| $items[] = $this->getImageCanvas( | ||
| $id, | ||
| $i, | ||
| $current, | ||
| $type, | ||
| $list, | ||
| $outline | ||
| ); | ||
| } else { | ||
| $items[] = $this->getAVCanvas( | ||
| $id, | ||
| $i, | ||
| $current, | ||
| $type, | ||
| $list, | ||
| $outline | ||
| ); |
There was a problem hiding this comment.
This might be a good place to use the ternary operator instead:
| if($type==='image'){ | |
| $items[] = $this->getImageCanvas( | |
| $id, | |
| $i, | |
| $current, | |
| $type, | |
| $list, | |
| $outline | |
| ); | |
| } else { | |
| $items[] = $this->getAVCanvas( | |
| $id, | |
| $i, | |
| $current, | |
| $type, | |
| $list, | |
| $outline | |
| ); | |
| $items[] = ($type==='image') | |
| ? $this->getImageCanvas( | |
| $id, | |
| $i, | |
| $current, | |
| $type, | |
| $list, | |
| $outline | |
| ) | |
| : $this->getAVCanvas( | |
| $id, | |
| $i, | |
| $current, | |
| $type, | |
| $list, | |
| $outline | |
| ); |
...though if you prefer to keep the if/else, that's fine too. However, the lack of space after your if and in front of your { suggests that the code needs to have composer fix run on it to clean up some minor style points.
2 participants
This is my first draft of a rewrite of the manifest generator code to bring it up to IIIF presentation standard 3.0